fix(Popup): render element as body children #302
Conversation
kuzhelov
requested review from
dvdzkwsk,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #302 +/- ##
==========================================
- Coverage 89.63% 89.49% -0.14%
==========================================
Files 62 62
Lines 1167 1171 +4
Branches 173 152 -21
==========================================
+ Hits 1046 1048 +2
- Misses 119 121 +2
Partials 2 2
Continue to review full report at Codecov.
|
| {open && this.renderPopupContent(rtl, accessibility)} | ||
| {open && | ||
| Popup.isBrowserContext && | ||
| createPortal(this.renderPopupContent(rtl, accessibility), document.body)} |
There was a problem hiding this comment.
This is probably more related to #282 but what's the reason to use React Portal directly instead of using stardust Portal component?
The main benefit of using stardust Portal component would be a reuse of onOutsideClick (close popup when user clicks outside the popup/trigger) and similar.
There was a problem hiding this comment.
+1, also stardust Portal component is going to have Focus Trap Zone, which can be used in Popup too, by just passing prop to enable it.
Otherwise, we would need to duplicate logic with focus trap in Popup component
There was a problem hiding this comment.
would rather like to address it by means of separate PR. The reason for that is, primarily, the following: apart from introducing this common functionality, the Portal component introduces quite a few additional conventions, such as render as direct body's children, etc - and there are not intuitive for the client given just the Portal's name and API - probably, it should be renamed first.
So, my intuition is to introduce the functionality that is necessary as a first thing, and after that generalize by making refactoring of Portal component - in a way of making this component itself more generic and properly translate its functionality and application strategies via its name and API (BodyPortal is something that comes to my head right now, but again, would rather like to see a clear picture first). More than that, now it seems that only these two aspects are addressed by Portal - and thus it is quite easy to introduce a dedicated function instead of component for that. One of the pros of this approach is that we will save one component from the Popup's rendered tree.
Please, let me know about your thoughts on that.
There was a problem hiding this comment.
If these changes are unblocking some other pull request, I am okay with them being merged and then the Portal can be replace with Stardust's Portal. In that way we can make progress on both aspects.
There was a problem hiding this comment.
have discussed with @sophieH29 - essentially, have agreed on the same plan. Lets wait on @miroslavstastny's comments and merge if there won't be anything critical.
kuzhelov
added
ready for merge
postponed
kuzhelov
removed
the
postponed
This PR fixes issue #294 by ensuring that
Popupcomponent's DOM tree is rendered asbody's child element.